-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace inline styles with css
prop in src/
folder
#201563
Conversation
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data_view_field_editor/public/components/preview/field_list/field_list.tsx
Outdated
Show resolved
Hide resolved
src/plugins/unified_search/public/query_string_input/add_filter_popover.tsx
Outdated
Show resolved
Hide resolved
b538080
to
de8278a
Compare
de8278a
to
111d4d1
Compare
111d4d1
to
c85dfef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security changes LGTM.
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#7489[❌] x-pack/test/functional/apps/lens/group6/config.ts: 0/25 tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review + quick local test ensure a subset of the components touched continue to render properly. LGTM 👍
src/plugins/chart_expressions/expression_legacy_metric/public/components/with_auto_scale.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/with_auto_scale.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
src/plugins/unified_search/public/query_string_input/query_bar_top_row.tsx
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/metric_value.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/metric_value.tsx
Outdated
Show resolved
Hide resolved
src/plugins/es_ui_shared/static/forms/docs/examples/fields_composition.mdx
Outdated
Show resolved
Hide resolved
src/plugins/presentation_util/public/components/expression_input/expression_input.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/metric_value.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/metric_value.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/with_auto_scale.tsx
Outdated
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_metric/public/components/metric_vis.tsx
Outdated
Show resolved
Hide resolved
@@ -38,7 +38,7 @@ export const SearchSessionName: React.FC<SearchSessionNameProps> = ({ name, edit | |||
justifyContent={'spaceBetween'} | |||
gutterSize={'none'} | |||
// padding to align with compressed input size | |||
style={{ paddingTop: 4, paddingBottom: 4 }} | |||
css={{ paddingTop: 4, paddingBottom: 4 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd replace static numbers with EUI tokens (e.g. euiTheme.size.xs
), but that's probably out of scope for this PR. Just mentioning it here to spread the knowledge/for the future
src/plugins/image_embeddable/public/components/image_viewer/image_viewer.tsx
Outdated
Show resolved
Hide resolved
src/plugins/presentation_util/public/components/expression_input/expression_input.tsx
Outdated
Show resolved
Hide resolved
src/plugins/vis_type_markdown/public/markdown_vis_controller.tsx
Outdated
Show resolved
Hide resolved
src/plugins/vis_types/timeseries/public/application/visualizations/views/top_n.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kibana management changes lgtm
db4cfc7
to
d3b9c47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this PR LGTM at this point! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data Discovery changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ES|QL changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared-ux lgtm! only static styles changes
f016661
to
7ff9521
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @albertoblaz |
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/12120160086 |
## Summary Part of the resolution of this issue: - elastic#149246 Removes the `style` prop in React components and elements to avoid using inline styles. Instead, it uses now the `emotion.css` prop to dynamically attach all styles to the native `class` attribute. ### Motivation Using inline styles at scale causes a performance penalty at rendering time. It's way more efficient to attach styles to a single or several classnames instead. ### How to review From [Emotion's official docs](https://emotion.sh/docs/css-prop#style-precedence): > [!NOTE] > Any component or element that accepts a `className` prop can also use the `css` prop. The styles supplied to the `css` prop are evaluated and the computed class name is applied to the `className` prop. Components that are safe to migrate from `style` to `css`: - React elements - React components exposed by EUI (they all support a `className` prop and the [Babel plugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is enabled throughout Kibana) Components where styling might break: - Custom component we wrote that currently don't accept a `className` prop. - Specific 3P components that pass in a `style` prop to our components i.e. `react-window` (it calculates the dynamic position of all virtualized elements and hides the ones outside the visible fold) - Specific 3P components that expect a `style` prop to be styled i.e. charts, etc. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ### Identify risks Main risk is breaking the UI by not applying or incorrectly applying the styling. This was explained in the "How to review" section above. The risk has **low severity** though. The mitigation plan will simply be reverting the commit asap not to affect production and test locally until it's fixed./ (cherry picked from commit 6aaecd0)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…x60; folder (#201563) (#202460) # Backport This will backport the following commits from `main` to `8.x`: - [Replace inline styles with `css` prop in `src/` folder (#201563)](#201563) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Alberto Blázquez","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-02T13:16:23Z","message":"Replace inline styles with `css` prop in `src/` folder (#201563)\n\n## Summary\r\n\r\nPart of the resolution of this issue: \r\n- https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the `style` prop in React components and elements to avoid using\r\ninline styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically attach all styles to the native `class` attribute.\r\n\r\n### Motivation\r\n\r\nUsing inline styles at scale causes a performance penalty at rendering\r\ntime. It's way more efficient to attach styles to a single or several\r\nclassnames instead.\r\n\r\n### How to review\r\n\r\nFrom [Emotion's official\r\ndocs](https://emotion.sh/docs/css-prop#style-precedence):\r\n\r\n> [!NOTE]\r\n> Any component or element that accepts a `className` prop can also use\r\nthe `css` prop. The styles supplied to the `css` prop are evaluated and\r\nthe computed class name is applied to the `className` prop.\r\n\r\nComponents that are safe to migrate from `style` to `css`:\r\n- React elements\r\n- React components exposed by EUI (they all support a `className` prop\r\nand the [Babel\r\nplugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is\r\nenabled throughout Kibana)\r\n\r\nComponents where styling might break:\r\n- Custom component we wrote that currently don't accept a `className`\r\nprop.\r\n- Specific 3P components that pass in a `style` prop to our components\r\ni.e. `react-window` (it calculates the dynamic position of all\r\nvirtualized elements and hides the ones outside the visible fold)\r\n- Specific 3P components that expect a `style` prop to be styled i.e.\r\ncharts, etc.\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n### Identify risks\r\n\r\nMain risk is breaking the UI by not applying or incorrectly applying the\r\nstyling. This was explained in the \"How to review\" section above. The\r\nrisk has **low severity** though. The mitigation plan will simply be\r\nreverting the commit asap not to affect production and test locally\r\nuntil it's fixed./","sha":"6aaecd0d8f4021d0b6dab350876367e56e1fa70c","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:ExpressionLanguage","release_note:skip","v9.0.0","backport:prev-minor","backport:version","v8.18.0"],"title":"Replace inline styles with `css` prop in `src/` folder","number":201563,"url":"https://github.com/elastic/kibana/pull/201563","mergeCommit":{"message":"Replace inline styles with `css` prop in `src/` folder (#201563)\n\n## Summary\r\n\r\nPart of the resolution of this issue: \r\n- https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the `style` prop in React components and elements to avoid using\r\ninline styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically attach all styles to the native `class` attribute.\r\n\r\n### Motivation\r\n\r\nUsing inline styles at scale causes a performance penalty at rendering\r\ntime. It's way more efficient to attach styles to a single or several\r\nclassnames instead.\r\n\r\n### How to review\r\n\r\nFrom [Emotion's official\r\ndocs](https://emotion.sh/docs/css-prop#style-precedence):\r\n\r\n> [!NOTE]\r\n> Any component or element that accepts a `className` prop can also use\r\nthe `css` prop. The styles supplied to the `css` prop are evaluated and\r\nthe computed class name is applied to the `className` prop.\r\n\r\nComponents that are safe to migrate from `style` to `css`:\r\n- React elements\r\n- React components exposed by EUI (they all support a `className` prop\r\nand the [Babel\r\nplugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is\r\nenabled throughout Kibana)\r\n\r\nComponents where styling might break:\r\n- Custom component we wrote that currently don't accept a `className`\r\nprop.\r\n- Specific 3P components that pass in a `style` prop to our components\r\ni.e. `react-window` (it calculates the dynamic position of all\r\nvirtualized elements and hides the ones outside the visible fold)\r\n- Specific 3P components that expect a `style` prop to be styled i.e.\r\ncharts, etc.\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n### Identify risks\r\n\r\nMain risk is breaking the UI by not applying or incorrectly applying the\r\nstyling. This was explained in the \"How to review\" section above. The\r\nrisk has **low severity** though. The mitigation plan will simply be\r\nreverting the commit asap not to affect production and test locally\r\nuntil it's fixed./","sha":"6aaecd0d8f4021d0b6dab350876367e56e1fa70c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201563","number":201563,"mergeCommit":{"message":"Replace inline styles with `css` prop in `src/` folder (#201563)\n\n## Summary\r\n\r\nPart of the resolution of this issue: \r\n- https://github.com/elastic/kibana/issues/149246\r\n\r\nRemoves the `style` prop in React components and elements to avoid using\r\ninline styles. Instead, it uses now the `emotion.css` prop to\r\ndynamically attach all styles to the native `class` attribute.\r\n\r\n### Motivation\r\n\r\nUsing inline styles at scale causes a performance penalty at rendering\r\ntime. It's way more efficient to attach styles to a single or several\r\nclassnames instead.\r\n\r\n### How to review\r\n\r\nFrom [Emotion's official\r\ndocs](https://emotion.sh/docs/css-prop#style-precedence):\r\n\r\n> [!NOTE]\r\n> Any component or element that accepts a `className` prop can also use\r\nthe `css` prop. The styles supplied to the `css` prop are evaluated and\r\nthe computed class name is applied to the `className` prop.\r\n\r\nComponents that are safe to migrate from `style` to `css`:\r\n- React elements\r\n- React components exposed by EUI (they all support a `className` prop\r\nand the [Babel\r\nplugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is\r\nenabled throughout Kibana)\r\n\r\nComponents where styling might break:\r\n- Custom component we wrote that currently don't accept a `className`\r\nprop.\r\n- Specific 3P components that pass in a `style` prop to our components\r\ni.e. `react-window` (it calculates the dynamic position of all\r\nvirtualized elements and hides the ones outside the visible fold)\r\n- Specific 3P components that expect a `style` prop to be styled i.e.\r\ncharts, etc.\r\n\r\n### Checklist\r\n\r\nCheck the PR satisfies following conditions. \r\n\r\nReviewers should verify this PR satisfies this list as well.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] This was checked for breaking HTTP API changes, and any breaking\r\nchanges have been approved by the breaking-change committee. The\r\n`release_note:breaking` label should be applied in these situations.\r\n- [x] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n### Identify risks\r\n\r\nMain risk is breaking the UI by not applying or incorrectly applying the\r\nstyling. This was explained in the \"How to review\" section above. The\r\nrisk has **low severity** though. The mitigation plan will simply be\r\nreverting the commit asap not to affect production and test locally\r\nuntil it's fixed./","sha":"6aaecd0d8f4021d0b6dab350876367e56e1fa70c"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Alberto Blázquez <[email protected]>
## Summary Part of the resolution of this issue: - elastic#149246 Removes the `style` prop in React components and elements to avoid using inline styles. Instead, it uses now the `emotion.css` prop to dynamically attach all styles to the native `class` attribute. ### Motivation Using inline styles at scale causes a performance penalty at rendering time. It's way more efficient to attach styles to a single or several classnames instead. ### How to review From [Emotion's official docs](https://emotion.sh/docs/css-prop#style-precedence): > [!NOTE] > Any component or element that accepts a `className` prop can also use the `css` prop. The styles supplied to the `css` prop are evaluated and the computed class name is applied to the `className` prop. Components that are safe to migrate from `style` to `css`: - React elements - React components exposed by EUI (they all support a `className` prop and the [Babel plugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is enabled throughout Kibana) Components where styling might break: - Custom component we wrote that currently don't accept a `className` prop. - Specific 3P components that pass in a `style` prop to our components i.e. `react-window` (it calculates the dynamic position of all virtualized elements and hides the ones outside the visible fold) - Specific 3P components that expect a `style` prop to be styled i.e. charts, etc. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ### Identify risks Main risk is breaking the UI by not applying or incorrectly applying the styling. This was explained in the "How to review" section above. The risk has **low severity** though. The mitigation plan will simply be reverting the commit asap not to affect production and test locally until it's fixed./
## Summary Part of the resolution of this issue: - elastic#149246 Removes the `style` prop in React components and elements to avoid using inline styles. Instead, it uses now the `emotion.css` prop to dynamically attach all styles to the native `class` attribute. ### Motivation Using inline styles at scale causes a performance penalty at rendering time. It's way more efficient to attach styles to a single or several classnames instead. ### How to review From [Emotion's official docs](https://emotion.sh/docs/css-prop#style-precedence): > [!NOTE] > Any component or element that accepts a `className` prop can also use the `css` prop. The styles supplied to the `css` prop are evaluated and the computed class name is applied to the `className` prop. Components that are safe to migrate from `style` to `css`: - React elements - React components exposed by EUI (they all support a `className` prop and the [Babel plugin](https://www.npmjs.com/package/@emotion/babel-preset-css-prop) is enabled throughout Kibana) Components where styling might break: - Custom component we wrote that currently don't accept a `className` prop. - Specific 3P components that pass in a `style` prop to our components i.e. `react-window` (it calculates the dynamic position of all virtualized elements and hides the ones outside the visible fold) - Specific 3P components that expect a `style` prop to be styled i.e. charts, etc. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) ### Identify risks Main risk is breaking the UI by not applying or incorrectly applying the styling. This was explained in the "How to review" section above. The risk has **low severity** though. The mitigation plan will simply be reverting the commit asap not to affect production and test locally until it's fixed./
Summary
Part of the resolution of this issue:
style
withemotion.css
#149246Removes the
style
prop in React components and elements to avoid using inline styles. Instead, it uses now theemotion.css
prop to dynamically attach all styles to the nativeclass
attribute.Motivation
Using inline styles at scale causes a performance penalty at rendering time. It's way more efficient to attach styles to a single or several classnames instead.
How to review
From Emotion's official docs:
Note
Any component or element that accepts a
className
prop can also use thecss
prop. The styles supplied to thecss
prop are evaluated and the computed class name is applied to theclassName
prop.Components that are safe to migrate from
style
tocss
:className
prop and the Babel plugin is enabled throughout Kibana)Components where styling might break:
className
prop.style
prop to our components i.e.react-window
(it calculates the dynamic position of all virtualized elements and hides the ones outside the visible fold)style
prop to be styled i.e. charts, etc.Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breaking
label should be applied in these situations.release_note:*
label is applied per the guidelinesIdentify risks
Main risk is breaking the UI by not applying or incorrectly applying the styling. This was explained in the "How to review" section above. The risk has low severity though. The mitigation plan will simply be reverting the commit asap not to affect production and test locally until it's fixed./